[None][infra] CBTS coverage data enhancement#16835
Conversation
…eteness, per-stage attribution Recover per-test coverage that was lost for inference-in-subprocess tests, and let the merged touch DB flag its own gaps so the selector can force-run anything that was not cleanly captured. - sitecustomize: periodic save is now unconditional so mpi4py.futures pool workers save before the pool is torn down at test end (their atexit save was lost); only the mpi-session patcher and marker poller remain daemon-gated. - cbts_plugin: record each test outcome via pytest_runtest_makereport, and count the pool workers each test spawns by patching MPIPoolExecutor.__init__. - cbts_pystart: each per-process file now carries proc_meta (stage) and, in the coordinator, test_meta (outcome + expected worker count). - pystart_report: merged touch gains a stage column; a merged test_meta (test, stage, outcome, expected_workers, saved_procs) exposes completeness; meta gains schema_version. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
|
/bot run --disable-fail-fast --stage-list "RTXPro6000D-PyTorch-1, RTXPro6000D-PyTorch-Post-Merge-1" |
WalkthroughCBTS coverage capture now records per-test outcomes, expected subprocess counts, and process stages. Tracker SQLite output and merged reports include completeness metadata, stage-aware touch entries, schema version 2, updated query guidance, and broader pipeline coverage eligibility. ChangesCBTS completeness tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cbts_plugin
participant sitecustomize
participant PyStartTracker
participant pystart_report
cbts_plugin->>sitecustomize: record outcome and expected workers
sitecustomize->>PyStartTracker: forward per-test metadata
sitecustomize->>PyStartTracker: periodically save coverage
pystart_report->>PyStartTracker: read staged SQLite outputs
pystart_report->>pystart_report: merge touch and test_meta rows
pystart_report->>pystart_report: compute CBTS completeness
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
jenkins/scripts/cbts/coverage_utils/cbts_plugin.py (1)
86-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the swallowed accounting errors.
except Exception: passhides hook/programming failures and leavesexpected_workersat zero, weakening completeness reporting. Catch known conversion errors separately and handle hook failures explicitly.As per coding guidelines, “Catch the narrowest possible exceptions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jenkins/scripts/cbts/coverage_utils/cbts_plugin.py` around lines 86 - 91, Update the worker-accounting logic around _sitecustomize_call to catch only expected max_workers conversion errors, such as TypeError or ValueError, while handling failures from note_expected_workers explicitly rather than swallowing them. Preserve the fallback of one worker for missing or invalid worker counts, and ensure hook failures are surfaced or otherwise handled according to the module’s established error behavior.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/scripts/cbts/coverage_utils/cbts_plugin.py`:
- Around line 115-119: Update the marker-polled sitecustomize watcher to also
call install_expected_workers_patch(), matching the existing pytest_configure
installation while retaining install_mpi_pool_patch. Ensure non-pytest
coordinators and subprocesses creating MPIPoolExecutor record expected workers
for merge accounting.
- Around line 147-149: Update the report filtering condition in the
outcome-recording logic to include non-passing teardown reports, alongside call
reports and non-passing setup reports. Ensure a failing or skipped teardown
passed to _sitecustomize_call("record_test_outcome", ...) overwrites the earlier
passed outcome for the same item.nodeid.
In `@jenkins/scripts/cbts/coverage_utils/cbts_pystart.py`:
- Around line 120-124: Update the fork-child reset logic in _after_fork_child()
to clear _outcomes and _expected alongside _data, ensuring save() does not
snapshot inherited coordinator metadata. Preserve normal parent-process metadata
and existing per-process coverage data behavior.
In `@jenkins/scripts/cbts/coverage_utils/pystart_report.py`:
- Around line 116-120: Update the batch construction in the _safe_rows
processing loop to store the stage-prefixed test key in touch.test, using the
existing stage and test values so staged inputs are persisted as “stage/test”.
Keep the remaining file, qualification, and stage columns unchanged, ensuring
coverage_selection/touch_db.py can derive the correct stage and nodeid.
---
Nitpick comments:
In `@jenkins/scripts/cbts/coverage_utils/cbts_plugin.py`:
- Around line 86-91: Update the worker-accounting logic around
_sitecustomize_call to catch only expected max_workers conversion errors, such
as TypeError or ValueError, while handling failures from note_expected_workers
explicitly rather than swallowing them. Preserve the fallback of one worker for
missing or invalid worker counts, and ensure hook failures are surfaced or
otherwise handled according to the module’s established error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 622941c6-7ae7-430f-8491-89e57b8dc8c8
📒 Files selected for processing (5)
jenkins/scripts/cbts/coverage_utils/README.mdjenkins/scripts/cbts/coverage_utils/cbts_plugin.pyjenkins/scripts/cbts/coverage_utils/cbts_pystart.pyjenkins/scripts/cbts/coverage_utils/pystart_report.pyjenkins/scripts/cbts/coverage_utils/sitecustomize.py
| def pytest_configure(config): # noqa: D401 - pytest hook | ||
| """Apply ``mpi_session`` monkeypatch with a compatibility guard.""" | ||
| """Apply the ``mpi_session`` env monkeypatch and the pool-worker accounting patch.""" | ||
| del config | ||
| install_mpi_pool_patch(raise_on_refactor=True) | ||
| install_expected_workers_patch() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Install expected-worker accounting in non-pytest coordinators too.
This patch is installed only by pytest_configure, while the marker-polled sitecustomize path installs only install_mpi_pool_patch. A service/subprocess that creates an MPIPoolExecutor therefore records no expected workers; merge treats that as zero and can incorrectly mark incomplete capture safe. Install install_expected_workers_patch() from that watcher as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jenkins/scripts/cbts/coverage_utils/cbts_plugin.py` around lines 115 - 119,
Update the marker-polled sitecustomize watcher to also call
install_expected_workers_patch(), matching the existing pytest_configure
installation while retaining install_mpi_pool_patch. Ensure non-pytest
coordinators and subprocesses creating MPIPoolExecutor record expected workers
for merge accounting.
| # The call phase is the test body; a non-passing setup is the test's effective outcome. | ||
| if report.when == "call" or (report.when == "setup" and report.outcome != "passed"): | ||
| _sitecustomize_call("record_test_outcome", item.nodeid, report.outcome) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record failing teardown outcomes.
A passing call phase is persisted as passed, but a subsequent failing/skipped teardown is ignored. Include non-passing teardown reports so these tests remain unsafe to skip.
Proposed fix
- if report.when == "call" or (report.when == "setup" and report.outcome != "passed"):
+ if report.when == "call" or (
+ report.when in {"setup", "teardown"} and report.outcome != "passed"
+ ):
_sitecustomize_call("record_test_outcome", item.nodeid, report.outcome)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The call phase is the test body; a non-passing setup is the test's effective outcome. | |
| if report.when == "call" or (report.when == "setup" and report.outcome != "passed"): | |
| _sitecustomize_call("record_test_outcome", item.nodeid, report.outcome) | |
| # The call phase is the test body; a non-passing setup is the test's effective outcome. | |
| if report.when == "call" or ( | |
| report.when in {"setup", "teardown"} and report.outcome != "passed" | |
| ): | |
| _sitecustomize_call("record_test_outcome", item.nodeid, report.outcome) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jenkins/scripts/cbts/coverage_utils/cbts_plugin.py` around lines 147 - 149,
Update the report filtering condition in the outcome-recording logic to include
non-passing teardown reports, alongside call reports and non-passing setup
reports. Ensure a failing or skipped teardown passed to
_sitecustomize_call("record_test_outcome", ...) overwrites the earlier passed
outcome for the same item.nodeid.
| def save(self): | ||
| # Write a per-process SQLite the downstream merge reads directly; uploaded compressed only. | ||
| snap = self._data.copy() # atomic shallow copy; each set snapshotted below | ||
| if not snap: | ||
| outcomes = dict(self._outcomes) | ||
| expected = dict(self._expected) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear coordinator metadata after a fork.
_after_fork_child() resets only _data; forked children inherit _outcomes and _expected. These snapshots then persist duplicated coordinator metadata, while merge sums expected_workers across files, inflating requirements for prior tests and producing false-incomplete results.
Proposed fix
def _after_fork_child(self):
# The child writes its own data file and rediscovers what it runs.
self._new_suffix()
self._data = {}
+ self._outcomes = {}
+ self._expected = {}
if self._active:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jenkins/scripts/cbts/coverage_utils/cbts_pystart.py` around lines 120 - 124,
Update the fork-child reset logic in _after_fork_child() to clear _outcomes and
_expected alongside _data, ensuring save() does not snapshot inherited
coordinator metadata. Preserve normal parent-process metadata and existing
per-process coverage data behavior.
| for test, file, qual in _safe_rows(fp): | ||
| seen_tests.add(test) | ||
| batch.append((test, file, qual, stage)) | ||
| if len(batch) >= 20000: | ||
| con.executemany("INSERT OR IGNORE INTO touch VALUES (?, ?, ?)", batch) | ||
| con.executemany("INSERT OR IGNORE INTO touch VALUES (?, ?, ?, ?)", batch) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the selector’s stage-prefixed test key.
The plugin supplies raw item.nodeid, and this insert keeps it raw. coverage_selection/touch_db.py derives (stage, nodeid) by splitting touch.test, so a node ID such as tests/... is misread as stage tests; per-stage selection breaks. Store f"{stage}/{test}" in touch.test for staged inputs, or update all selector consumers atomically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jenkins/scripts/cbts/coverage_utils/pystart_report.py` around lines 116 -
120, Update the batch construction in the _safe_rows processing loop to store
the stage-prefixed test key in touch.test, using the existing stage and test
values so staged inputs are persisted as “stage/test”. Keep the remaining file,
qualification, and stage columns unchanged, ensuring
coverage_selection/touch_db.py can derive the correct stage and nodeid.
|
PR_Github #61548 [ run ] triggered by Bot. Commit: |
|
/bot kill |
…ction Temporary: collect CBTS per-test coverage on a manual pre-merge run to validate the completeness/attribution changes. Revert before merge (restore the env.JOB_NAME PostMerge gate on testFilter[cbts_coverage]). Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
|
PR_Github #61552 [ kill ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --stage-list "RTXPro6000D-PyTorch-1" |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 342-345: Restore the JOB_NAME post-merge condition in the
CBTS_COVERAGE assignment within testFilter, combining ENABLE_CBTS_COVERAGE with
the existing PostMerge job-name regex. Remove the temporary test-only assignment
so isCbtsStage() enables coverage only for eligible post-merge pipelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54f8db78-05d6-4a1f-857c-d6b1c5d84bcc
📒 Files selected for processing (1)
jenkins/L0_MergeRequest.groovy
| // TEST ONLY (revert before merge): the official-post-merge gate is dropped so coverage is | ||
| // collected on a manual run to validate collection. Restore the JOB_NAME gate before merging: | ||
| // testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE && (env.JOB_NAME ==~ /.*PostMerge.*/) | ||
| testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Restore the post-merge gate before merging.
ENABLE_CBTS_COVERAGE is true, so this assignment enables CBTS coverage for every eligible pre-merge pipeline. Downstream isCbtsStage() consumes this flag directly, causing non-excluded single-GPU pre-merge stages to collect coverage, contrary to the documented post-merge-only contract. The comments do not enforce the required rollback.
Proposed fix
- testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE
+ testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE && (env.JOB_NAME ==~ /.*PostMerge.*/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TEST ONLY (revert before merge): the official-post-merge gate is dropped so coverage is | |
| // collected on a manual run to validate collection. Restore the JOB_NAME gate before merging: | |
| // testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE && (env.JOB_NAME ==~ /.*PostMerge.*/) | |
| testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE | |
| // TEST ONLY (revert before merge): the official-post-merge gate is dropped so coverage is | |
| // collected on a manual run to validate collection. Restore the JOB_NAME gate before merging: | |
| // testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE && (env.JOB_NAME ==~ /.*PostMerge.*/) | |
| testFilter[(CBTS_COVERAGE)] = ENABLE_CBTS_COVERAGE && (env.JOB_NAME ==~ /.*PostMerge.*/) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jenkins/L0_MergeRequest.groovy` around lines 342 - 345, Restore the JOB_NAME
post-merge condition in the CBTS_COVERAGE assignment within testFilter,
combining ENABLE_CBTS_COVERAGE with the existing PostMerge job-name regex.
Remove the temporary test-only assignment so isCbtsStage() enables coverage only
for eligible post-merge pipelines.
|
PR_Github #61548 [ run ] completed with state |
|
PR_Github #61552 [ kill ] completed with state |
|
PR_Github #61554 [ run ] triggered by Bot. Commit: |
|
PR_Github #61554 [ run ] completed with state |
…e running test MPI-pool workers saved their coverage (periodic save) but recorded it under the empty context: they don't run the pytest plugin and their inherited CBTS_TEST_ID is stale/empty at spawn, and the marker poller was gated off for them. Run the marker poller for pool workers too (not _is_pytest_main) so they follow the per-test marker file and attribute coverage to the test they serve, the same way long-lived non-pytest processes do. Signed-off-by: Ivy Zhang <25222398+crazydemo@users.noreply.github.com>
|
/bot run --disable-fail-fast --stage-list "RTXPro6000D-PyTorch-1" |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/scripts/cbts/coverage_utils/sitecustomize.py`:
- Line 185: Update the non-pytest marker handling guarded by _is_pytest_main to
use a private per-run temporary directory instead of a predictable global /tmp
path. Ensure cbts_plugin.py marker writes use atomic file replacement rather
than open(..., "w"), preserving marker polling while preventing symlink
redirection and partial/truncated marker contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 614c8544-4597-47cb-a92d-8fe0658afea6
📒 Files selected for processing (1)
jenkins/scripts/cbts/coverage_utils/sitecustomize.py
| # Pool workers and long-lived non-pytest processes follow the per-test marker file to switch | ||
| # context; a pool worker would otherwise record every test it serves under one inherited (often | ||
| # empty) context. The outer pytest switches context via the plugin instead. | ||
| if not _is_pytest_main: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid a predictable shared /tmp marker path.
With marker polling now enabled for non-pytest processes, the fallback path can be manipulated on shared CI hosts. cbts_plugin.py writes the same path with open(..., "w"); a same-user process could inject node IDs or use a symlink to redirect and truncate another file. Use a private per-run directory and atomic marker-file replacement instead of a predictable global /tmp path.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 185-185: Do not hardcode temporary file or directory names
Context: "/tmp/cbts/current_test.txt"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jenkins/scripts/cbts/coverage_utils/sitecustomize.py` at line 185, Update the
non-pytest marker handling guarded by _is_pytest_main to use a private per-run
temporary directory instead of a predictable global /tmp path. Ensure
cbts_plugin.py marker writes use atomic file replacement rather than open(...,
"w"), preserving marker polling while preventing symlink redirection and
partial/truncated marker contents.
Source: Linters/SAST tools
|
PR_Github #61568 [ run ] triggered by Bot. Commit: |
|
PR_Github #61568 [ run ] completed with state |
2 participants
Dev Engineer Review
proc_meta(stage) andtest_meta(test outcome, expected worker count, andsaved_procs) and bumping the reporting schema toschema_version: "2".note_expected_workersvia anMPIPoolExecutorpatch), switch test context withsitecustomize, and persist non-passing outcomes (pytest_runtest_makereport→record_test_outcome).(test, file, qualname, stage)intouch, add/mergetest_meta, and compute completeness usingsaved_procs < expected_workers + 1(unsafe-to-skip only when passed and coverage is fully saved).(file, stage)querying and tightened “(test, stage) safe to skip” criteria).ENABLE_CBTS_COVERAGEinsetupPipelineEnvironment.QA Engineer Review
No test changes.
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.